perf(ttx): batch concurrent finality status lookups; remove dead transaction filter#1896
perf(ttx): batch concurrent finality status lookups; remove dead transaction filter#1896hstarorg wants to merge 5 commits into
Conversation
AcceptTxInDBsFilter.Accept issued up to two serial DB round trips per call, and FSC can have several such calls in flight concurrently while committing a block. Add GetStatuses(ctx, txIDs) to ttxdb/auditdb StoreService, and a statusBatcher that coalesces concurrent Accept lookups within a 300us window into a single batched query per store, instead of one query pair per transaction. Fixes LFDT-Panurus#1872 Signed-off-by: hstarorg <jayhu@sign.global>
require.NoError/ErrorIs must run on the test's main goroutine (go-require) and error assertions should use require, not assert (require-error). Fixes the 4 golangci-lint testifylint findings in filter_test.go and status_batcher_test.go from the PR LFDT-Panurus#1896 CI run. Signed-off-by: hstarorg <jayhu@sign.global>
|
Hi @hstarorg , thanks for submitting this 🙏 |
|
Hi @hstarorg , what's the performance improvement that you have seen with this patch? |
Heads-up: this filter currently isn't reachable from FSC's committerBefore merging — I want to flag something I found while re-reviewing, since it changes what this PR actually delivers at runtime.
transactionFilter, err := n.filterProvider.New(tmsID)
...
committer := n.ch.Committer()
if err := committer.AddTransactionFilter(transactionFilter); err != nil {— were removed from Practical effect: This predates this PR and isn't something it needs to fix by itself, but it seems worth deciding explicitly before merge:
Happy to send the follow-up PR restoring the registration if that's the preferred path. |
Note: the current caller path has that registration removed |
Understood, will do 🙏 |
|
Hi @hstarorg , super interesting. So, the reliance on the FSC's fabric's commit pipeline was removed because that path was too slow. We should not restore therefore it and I would suggest to remove the unused code. Nevertheless, it might be that your patch might still be helpful in |
|
Hi @hstarorg , any follow up on this? Thanks 🙏 |
Thanks for the pointer — that makes sense. I'll rework this PR in that direction: drop the unused Will update the PR shortly. |
Per review feedback on LFDT-Panurus#1896: the FSC commit-pipeline transaction filter path was intentionally removed in cd7e7a4 (LFDT-Panurus#1200) and should not be restored, so AcceptTxInDBsFilter, AcceptTxInDBFilterProvider, TransactionFilterProvider and the inert filterProvider plumbing in the fabric/fabricx network drivers and the dig container are deleted. The status batching is re-targeted at token/services/ttx/finality.go instead: the wrapper providers behind dep.GetTransactionDB/GetAuditDB now wrap each store service in a per-TMS batching decorator, so single-tx GetStatus lookups from concurrent finality views are coalesced into one batched GetStatuses query per 300us window. finality.go itself is unchanged. The batcher honors caller context cancellation while waiting. Signed-off-by: hstarorg <jayhu@sign.global>
hstarorg
changed the title
|
@adecaro PR updated as discussed:
One note: the batched query runs on |
|
Hi @hstarorg , thanks for this effort. I'll review ASAP 🙏 Do you have already information about the impact of these changes on the performance of your deployment? |
2 participants
Reworked per review discussion (#1896 (comment) and below): the FSC commit-pipeline transaction filter path was intentionally removed in cd7e7a4 (#1200) and should not be restored, so this PR now does two things instead of optimizing the filter.
1. Remove the dead transaction-filter code
AcceptTxInDBsFilter,AcceptTxInDBFilterProvider,TransactionFilterProvider, and the inertfilterProviderplumbing threaded through the fabric/fabricx network drivers and the dig container are deleted. Nothing consumed them since theAddTransactionFilterregistration was removed in cd7e7a4.2. Batch concurrent status lookups in the ttx finality path
Each finality view queries ttxdb and auditdb for transaction status (initial known-check plus periodic polling), so under concurrent load many single-tx
GetStatusqueries hit the same stores. This PR addsGetStatuses(ctx, txIDs)to the ttxdb/auditdb store services and wraps each store in a per-TMS batching decorator behind thedep.GetTransactionDB/GetAuditDBproviders: single-tx lookups landing within a 300µs window are coalesced into one batched query per store.finality.goitself is unchanged — the batching is transparent at the provider layer.Notes:
Unknownwith no error; all finality call sites treat that identically to the old per-tx error path.context.Background()since it serves multiple callers; each waiter still honors its own context while waiting.Refs #1872